Skip to content

refactor: extract latest-leaf named backups facade#301

Closed
ndycode wants to merge 1 commit intorefactor/pr3-storage-save-flagged-entryfrom
refactor/pr3-storage-named-backups-latest-leaf
Closed

refactor: extract latest-leaf named backups facade#301
ndycode wants to merge 1 commit intorefactor/pr3-storage-save-flagged-entryfrom
refactor/pr3-storage-named-backups-latest-leaf

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 22, 2026

Summary

  • extract the getNamedBackups facade out of lib/storage.ts on top of the latest storage leaf
  • keep named backup listing behavior unchanged while slimming the storage facade on the active chain
  • add a focused test for the new named backups entry helper

Validation

  • npm run typecheck
  • npm run lint -- lib/storage/named-backups-entry.ts lib/storage.ts test/named-backups-entry.test.ts
  • npm run test -- test/named-backups-entry.test.ts

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr extracts the getNamedBackups wiring out of lib/storage.ts into a new lib/storage/named-backups-entry.ts facade, following the same leaf-entry DI pattern used by restoreAccountsFromBackupEntry and siblings. behavior is unchanged; storage.ts slims down by delegating to the new helper.

  • new file lib/storage/named-backups-entry.ts: thin facade accepting injected getStoragePath, collectNamedBackups, loadAccountsFromPath, and logDebug — all forwarded directly to collectNamedBackups, no logic added
  • lib/storage.ts: getNamedBackups now delegates to getNamedBackupsEntry; the call-site wiring is identical to the pre-refactor inline call
  • test/named-backups-entry.test.ts: single focused vitest case asserting the wiring — happy path only; non-empty result passthrough is unexercised
  • circular type import: named-backups-entry.ts imports NamedBackupSummary from ../storage.js rather than from ./named-backups.js where it is actually defined, creating a type-level cycle in the module graph; harmless at runtime but worth fixing
  • lib/AGENTS.md / lib/storage directory listing not updated to include the new file — minor maintenance gap

Confidence Score: 5/5

  • safe to merge — pure refactor, behavior unchanged, only non-blocking P2 style issues remain
  • the change is a straightforward delegation extraction with no logic delta. the only issues are a type-only circular import (erased at compile time, no runtime effect) and a single test case where a second passthrough assertion would be cleaner. both are P2 follow-ups, not blockers.
  • lib/storage/named-backups-entry.ts — fix the NamedBackupSummary import source

Important Files Changed

Filename Overview
lib/storage/named-backups-entry.ts new thin facade extracting getNamedBackups wiring; clean DI shape matches the existing named-backups.ts signature — only issue is importing NamedBackupSummary from ../storage.js instead of ./named-backups.js, creating a type-level circular reference
lib/storage.ts getNamedBackups now delegates to getNamedBackupsEntry; wiring is correct, all deps forwarded, behavior unchanged
test/named-backups-entry.test.ts covers the wiring/happy-path only with a single vi.fn test; non-empty passthrough and error-propagation cases are unexercised

Sequence Diagram

sequenceDiagram
    participant caller as caller
    participant storage as lib/storage.ts<br/>getNamedBackups()
    participant entry as lib/storage/named-backups-entry.ts<br/>getNamedBackupsEntry()
    participant collector as lib/storage/named-backups.ts<br/>collectNamedBackups()

    caller->>storage: getNamedBackups()
    storage->>entry: getNamedBackupsEntry({ getStoragePath, collectNamedBackups, loadAccountsFromPath, logDebug })
    entry->>entry: params.getStoragePath()
    entry->>collector: collectNamedBackups(storagePath, { loadAccountsFromPath, logDebug })
    collector-->>entry: NamedBackupSummary[]
    entry-->>storage: NamedBackupSummary[]
    storage-->>caller: NamedBackupSummary[]
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/storage/named-backups-entry.ts
Line: 1

Comment:
**circular type import — prefer direct source**

`NamedBackupSummary` is defined in `./named-backups.ts`, but this file imports it from `../storage.js`. that creates a logical type-level cycle: `storage.ts``named-backups-entry.ts``storage.ts`. `import type` is erased at compile time so there's no runtime issue, but the cycle still pollutes the module graph and can confuse tools like `madge`. import directly from the definition site to break the cycle:

```suggestion
import type { NamedBackupSummary } from "./named-backups.js";
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/named-backups-entry.test.ts
Line: 6-22

Comment:
**missing non-empty passthrough coverage**

the single test only exercises the `[]` return path. given the 80% vitest coverage threshold enforced on this codebase, consider adding a second case where `collectNamedBackups` resolves to a non-empty `NamedBackupSummary[]` and asserting the result is forwarded unchanged — verifies the return value isn't accidentally dropped or mutated. error-propagation from `collectNamedBackups` throwing is also unexercised, though for this thin a wrapper that's optional.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "refactor: extract la..."

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 22, 2026

Warning

Rate limit exceeded

@ndycode has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 24 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2d73cef2-f0d3-48d8-86fa-446f1dfcbc90

📥 Commits

Reviewing files that changed from the base of the PR and between 6b594b9 and 563ba19.

📒 Files selected for processing (3)
  • lib/storage.ts
  • lib/storage/named-backups-entry.ts
  • test/named-backups-entry.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/pr3-storage-named-backups-latest-leaf
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/pr3-storage-named-backups-latest-leaf

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@ndycode
Copy link
Owner Author

ndycode commented Mar 23, 2026

Closing because this work is now included in main via #318 and #319.

@ndycode ndycode closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant